Skip to content

Surface scanner version on security tab#18

Open
mgoldsborough wants to merge 2 commits intomainfrom
feat/surface-scanner-version
Open

Surface scanner version on security tab#18
mgoldsborough wants to merge 2 commits intomainfrom
feat/surface-scanner-version

Conversation

@mgoldsborough
Copy link
Contributor

Summary

  • Extract scanner_version from the scan report's scan metadata in the API transform layer
  • Add scanner_version field to all SecurityScan Zod schemas (source + generated copies)
  • Display scanner version in the security report subtitle line (e.g. "Scanner v0.4.2")
  • Only renders when present, so older scans without it display unchanged

Test plan

  • Verify typecheck/lint pass (pnpm typecheck && pnpm lint)
  • Check a bundle with a completed scan — scanner version should appear after the scan date
  • Check a bundle scanned before this field existed — should render without the scanner version gracefully

Extract scanner_version from the scan report metadata in the API
response and display it on the security tab next to the scan date.
@mgoldsborough mgoldsborough requested review from a team and JoeCardoso13 February 21, 2026 02:33
JoeCardoso13
JoeCardoso13 previously approved these changes Feb 21, 2026
Copy link
Collaborator

@JoeCardoso13 JoeCardoso13 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Findings

All low-severity — none are blocking. Most are pre-existing patterns rather than problems introduced by this PR.

1. Duplicated "generated" schemas are manually maintained (Low / Observation)

The schemas in apps/registry/src/schemas/generated/api-responses.ts and apps/web/src/schemas/generated/api-responses.ts appear to be copies of packages/schemas/src/api-responses.ts. The directory name generated/ implies these should be auto-generated, but this PR updates them by hand. If there is a codegen step, it should be run instead; if there isn't, the generated/ naming is misleading and risks drift over time.

Suggested improvement (optional): If these are auto-generated, both the registry and web apps should import from or generate against the single source of truth in packages/schemas/. If they're not auto-generated, rename the generated/ directories to something like shared/ or copied/ to avoid the misleading implication and reduce surprise for future contributors.

2. Type assertion on report.scan — no runtime validation (Low)

const scanMeta = report?.['scan'] as Record<string, unknown> | undefined;
const scannerVersion = (scanMeta?.['scanner_version'] as string) ?? null;

This uses as casts to extract the value. If scanner_version is present but not a string (e.g. a number), it would silently pass through and could produce unexpected UI output like "Scanner v[object Object]". Consistent with the existing pattern used for other fields in this file, but a typeof guard would be more robust.

Suggested hardening (optional):

const raw = scanMeta?.['scanner_version'];
const scannerVersion = typeof raw === 'string' ? raw : null;

3. Manual SecurityScan interface in the component duplicates the Zod schema type (Low / Observation)

SecurityReportSection.tsx defines its own SecurityScan TypeScript interface rather than deriving it from the Zod schema via z.infer<typeof SecurityScanSchema>. This means every schema change requires a parallel manual edit to the interface. Consistent with existing code, but worth noting.

Suggested improvement (optional): Replace the hand-written interface with z.infer<typeof SecurityScanSchema> (or a pick/partial of it) so the component stays in sync with the schema automatically. That would have made this PR one fewer file to touch.

4. No unit test for the new field extraction (Low)

There's no test covering the new extraction logic (what happens when report.scan.scanner_version is present, missing, or a non-string value). The existing test suite passes, but there's no coverage of this specific path.

Suggested improvement (optional): Add a small test case in the packages route tests that asserts scanner_version is surfaced when present in the report metadata and falls back to null when absent. This would also serve as a safety net for the type-assertion concern in item #2.

Extract extractScannerVersion into a testable utility and add 6
unit tests covering present, missing, null, and malformed report data.
@mgoldsborough mgoldsborough added qa-reviewed QA review completed with no critical issues pkg/web Web UI (app.mpak.dev) pkg/registry Registry API server pkg/schemas Shared schemas (@nimblebrain/mpak-schemas) labels Feb 26, 2026
Copy link
Collaborator

@JoeCardoso13 JoeCardoso13 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

APPROVE

Clean, well-structured feature addition. Schema propagation is consistent across all three locations, the utility is properly extracted and tested, and backward compatibility is maintained via nullable().optional(). CI is green.

Two minor observations (neither blocking):

  1. extractScannerVersion type safety — The as string cast + ?? null only catches null/undefined. A non-string truthy value (e.g. a number) or an empty string would pass through, which subtly diverges from the docstring's "null if missing/malformed" contract. Consider typeof guard + || null to cover both gaps. No visible bug since the UI's && check suppresses falsy values and the scanner controls this field.

  2. Test gap for empty string — No test covers { scan: { scanner_version: "" } }. Adding one would document the expected behavior for that edge case.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

pkg/registry Registry API server pkg/schemas Shared schemas (@nimblebrain/mpak-schemas) pkg/web Web UI (app.mpak.dev) qa-reviewed QA review completed with no critical issues

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants